fix(acp): reject unattended permission requests - #4609
Conversation
Default managed sessions to dontAsk and reject or cancel permission prompts instead of selecting allow_once. Explicit owner-selected non-interactive modes remain available. Co-authored-by: Jordan Mecom <jm@squareup.com> Signed-off-by: Jordan Mecom <jm@squareup.com>
Co-authored-by: Jordan Mecom <jm@squareup.com> Signed-off-by: Jordan Mecom <jm@squareup.com>
The permission tests re-implemented the `reject_once` lookup in the test body rather than calling the code under test, so they would all still pass if the harness went back to selecting `allow_once`. They could not call it directly: `handle_permission_request` is a method on `AcpClient`, which owns a live `Child` and its stdio pipes. Extract the choice into `permission_denial_response` and point the tests at it. No behaviour change. This covers the cancelled fallback, which had no test despite being the fail-closed backstop for adapters that offer no `reject_once`, plus the empty-option-list and missing-`optionId` edges. Also drops `find_allow_once_returns_none_when_absent`, which asserted a property of a search no production path performs any more. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Eli Foster <efoster@squareup.com>
elifoster-block
left a comment
elifoster-block
left a comment
There was a problem hiding this comment.
LGTM - expanded test coverage and removed an unused function.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Security review by Carl, acting on Wes’s behalf.
I found no blocking issue at head 16fff4dac4370e1a260739d59cdf7b1dda124e4e.
The patch closes both authorization paths that mattered: it removes bypassPermissions from the accepted configuration entirely and changes both ACP response loops to route session/request_permission through one fail-closed decision. That decision selects only an offered reject_once; if none exists it returns the protocol-level cancelled outcome. It never selects allow_once or allow_always, and malformed reject options tear the turn down rather than approving.
The default is now dontAsk. If an adapter does not advertise/support that mode, the harness still remains safe because every interactive request is rejected locally. Explicit modes such as acceptEdits remain operator-selected preauthorization, not an unattended prompt bypass.
The new tests exercise the production decision helper, including allow options offered alongside rejection, no rejection option, an empty list, malformed rejection, and JSON-RPC string IDs. Exact-head CI is green, including unit tests, Rust lint, security, desktop and relay/backend E2E, cross-compiles, DCO, Semgrep, and zizmor.
Operational consequence, intentionally fail-closed: managed desktop agents have no in-app permission prompt, so an operation requiring interactive approval will fail rather than pause for approval.
3 participants
This change removes the ACP permission-bypass mode, defaults managed sessions to
dontAsk, and answers permission requests withreject_onceor cancellation in both ACP read loops.Unattended operations that require interactive approval now fail closed instead of being silently authorized. Explicit non-interactive modes that do not bypass a permission request remain available.
Both layers have to change together:
apply_permission_modetreats an unsupported mode and a failedset_config_optionas non-fatal by design, so a request can still reach the harness even in a non-interactive mode. RemovingbypassPermissionsfrom the enum rather than only changing the default means the mode cannot be restored by configuration alone.The scope of the guarantee is that
buzz-acpnever grants approval. An agent that pre-authorizes tools in its own configuration (for example Claude Code'ssettings.json) still runs them without asking, which is outside this harness.Testing
env -u BUZZ_ACP_LAZY_POOL bin/cargo test -p buzz-acpat16fff4d: 671 library tests and 9 integration tests passedcargo clippy -p buzz-acp --all-targets -- -D warningsandcargo fmt -p buzz-acp -- --check: cleangit diff --check origin/main...codex/security-acp-shell-auto-approvalThe permission tests previously re-implemented the
reject_oncelookup in the test body instead of calling the code under test, so they would have passed unchanged if the harness went back to selectingallow_once. They could not call it directly, becausehandle_permission_requestis a method onAcpClient, which owns a liveChildand its stdio pipes. The choice is now a free function,permission_denial_response, and the tests exercise it:reject_oncepreferred over offered allow options, the cancelled fallback when noreject_onceexists, an empty option list, and areject_oncemissing itsoptionId. The cancelled fallback had no coverage before despite being the fail-closed backstop.Operator notes
BUZZ_ACP_PERMISSION_MODE=bypassPermissionsno longer parses, so a process configured with it fails to start rather than silently downgrading.dontAsk. The desktop has no permission prompt, so operations needing approval now fail with no in-app way to approve them.Originating Buzz thread:
buzz://message?channel=3928fe05-df61-4b5d-b9c7-d623b9b10ea1&id=3c6c02312f763fbe0d2bfc33a6c1a362f91d0354f3d18b039cf7a0558c1439d1